Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add TLS support #48

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

brettmc
Copy link

@brettmc brettmc commented Jun 7, 2018

This adds the ability to connect to TLS-protected queue brokers, via the 'protocol' option. Update affected unit tests, add functional test (verified against activemq v5.15.0). Also document how to run the functional tests

This adds the ability to connect to TLS-protected queue brokers, via the 'protocol' option. Update affected unit tests, add functional test (verified against activemq v5.15.0). Also document how to run the functional tests
@brettmc
Copy link
Author

brettmc commented Jun 7, 2018

...and those travis failures are totally not my fault. It looks like it's having networking issues?

@WyriHaximus
Copy link
Member

...and those travis failures are totally not my fault. It looks like it's having networking issues?

I'll kick the build a few times until it passes 👍

tree dependency is not found in trusty - two options to fix: 'apt update' before installing tree, or don't install it. I chose the latter because it does not look like it is being used
I was able to somewhat replicate the activemq install issue with the ubuntu:trusty docker image, and an 'apt update' resolved it
@brettmc
Copy link
Author

brettmc commented Jan 22, 2019

Hello maintainers. Is there anything I can do to help move this PR (and 49) along?

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@ghost
Copy link

ghost commented Oct 7, 2019

@clue 🏓

Copy link
Contributor

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brettmc Thanks for your PR, can you add (or think of) a test to verify this works as expected? 👍

In the future, this should be changed to use react/socket to asynchronously create the connection (#19), but let's keep this for a separate ticket 👍

@skaffille skaffille mentioned this pull request Oct 18, 2019
@brettmc
Copy link
Author

brettmc commented Oct 23, 2019

@jsor I can think of a way. I'll make some time to try to enable TLS on one of the stomp providers configured in the travis tests. If I can do that, I should be able to add a new provider (or extend the tests) to test that secure connections work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants